Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker #2278

Merged
merged 4 commits into from
Aug 8, 2022

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Jul 27, 2022

What I did

Fix issues:

  1. upgrade-docker shall not check STATE DB for those container which do not support warm mode
  2. rollback-docker sometimes cannot really rollback the container because it cannot get the correct docker image tag

How I did it

  1. for containers do not support warm mode, ignore STATE_DB check
  2. use another command to get docker image tag

How to verify it

Manual test.

UT is not created for this change because there are many docker command running for these two CLIs, thus:

  1. Mock so many docker command causes the test case very tricky. The case would be even more complicated than the CLIs.
  2. UT cannot really test the command in case of so many docker command

Instead, a new sonic-mgmt test case is on the way to cover these two CLI. Will create PR for the sonic-mgmt new test case later.

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

stepanblyschak
stepanblyschak previously approved these changes Jul 27, 2022
@yxieca
Copy link
Contributor

yxieca commented Jul 28, 2022

@Junchao-Mellanox can you add unit test?

@Junchao-Mellanox
Copy link
Collaborator Author

Hi Ying, these two CLIs runs a lot docker/sonic commands which makes it hard to be unit tested. I have a sonic-mgmt PR to cover the test: sonic-net/sonic-mgmt#6054 . Would that be ok?

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @yxieca , could you please provide your comment?

@@ -31,6 +31,8 @@ def run_command(command):
if proc.returncode != 0:
sys.exit(proc.returncode)

return out.rstrip("\n")
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return out.rstrip("\n")

To make coverage easier, do you really need this change? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I will rollback line 942, I don't need this change any more.

@@ -938,7 +939,8 @@ def rollback_docker(container_name):
version_tag = ""
for id in image_id_all:
if id != image_id_previous:
version_tag = get_docker_tag_name(id)
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_docker_tag_name

you can change the implementation of get_docker_tag_name(), and this function should be easy to unit test, you still need some mock. #Closed

Copy link
Collaborator Author

@Junchao-Mellanox Junchao-Mellanox Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi qi, thanks for the suggestion. I would to rollback this change because I found the real issue is in sonic build system. Here is my analyze:

During my previous manual test with a local image, I found get_docker_tag_name() cannot return the correct tag name of docker. The return value of get_docker_tag_name() does not equal to output of docker images.

However, I re-visit the logic today, and I found:

  1. get_docker_tag_name() tries to get the docker tag by querying a docker label specified by sonic make file.
cmd = "docker inspect --format '{{.ContainerConfig.Labels.Tag}}' " + image
  1. This label is added in slave.mk. See https://github.com/sonic-net/sonic-buildimage/blob/736c739bf492a73c60cfca000b853c040f53a104/slave.mk#L895
	docker build --squash --no-cache \
...
		--label Tag=$(SONIC_IMAGE_VERSION) \
  1. The real docker tag is added in build_debian.sh. See https://github.com/sonic-net/sonic-buildimage/blob/736c739bf492a73c60cfca000b853c040f53a104/files/build_templates/sonic_debian_extension.j2#L697
sudo LANG=C DOCKER_HOST="$DOCKER_HOST" chroot $FILESYSTEM_ROOT docker tag {{imagename}}:latest {{imagename}}:"${SONIC_IMAGE_VERSION}"

In theory, item#2 and item#3 shall get the same value of ${SONIC_IMAGE_VERSION}. But it doesn't. I got from build log:

  1. For item#2, it prints:
docker-database.gz.log:Step 23/25 : LABEL Tag=syslog-rate-limit.0-dirty-20220804.115836
  1. For item#3, it prints:
sonic-mellanox.bin.log:+ sudo LANG=C DOCKER_HOST= chroot ./fsroot-mellanox docker tag docker-database:latest docker-database:syslog-rate-limit.0-dirty-20220804.165951

As you can see, value of SONIC_IMAGE_VERSION is different in the same build. I suppose SONIC_IMAGE_VERSION value shall always be the same in the same build.

The issue is probably related to the timestamp in the SONIC_IMAGE_VERSION. However, I found timestamp is not part of SONIC_IMAGE_VERSION in official image. So, maybe we can ignore this issue.

if cleanup_image:
# Unless requested, the previoud docker image will be preserved
for id in image_id_all:
if id != image_id_latest and id == image_id_previous:
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id != image_id_latest and id == image_id_previous

This check is weird, seems like image_id_latest != image_id_previous, and not related to the loop var. Could you double check? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 5, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

qiluo-msft
qiluo-msft previously approved these changes Aug 5, 2022
@liat-grozovik
Copy link
Collaborator

@qiluo-msft could you please help to approve again following the improved coverage?

@qiluo-msft qiluo-msft merged commit bcf36eb into sonic-net:master Aug 8, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Aug 9, 2022
Update sonic-utilities submodule pointer to include the following:
* Fix GCU bug when backend service modifying config ([sonic-net#2295](sonic-net/sonic-utilities#2295))
* Fix issues for sonic_installer upgrade-docker and sonic_installer rollback-docker ([sonic-net#2278](sonic-net/sonic-utilities#2278))
* [crm] add checking for CRM interval range ([sonic-net#2293](sonic-net/sonic-utilities#2293))
* Fix the issue that sonic_platform is not installed on vs image ([sonic-net#2300](sonic-net/sonic-utilities#2300))
* Add FEC correctable and uncorrectable port stats ([sonic-net#2027](sonic-net/sonic-utilities#2027))
* Add CLI to configure YANG config validation ([sonic-net#2147](sonic-net/sonic-utilities#2147))
* Add override testcase to verify removal ([sonic-net#2288](sonic-net/sonic-utilities#2288))
* Fix version in db_migrator  for  ([sonic-net#2289](sonic-net/sonic-utilities#2289))
* [intfutil] Check whether the FEC mode is supported on the platform before configuring it to CONFIG_DB ([sonic-net#2223](sonic-net/sonic-utilities#2223))
* Transfer organization from Azure to sonic-net ([sonic-net#2284](sonic-net/sonic-utilities#2284))
* [watermarkstat] Fix CLI script for unconfigured PG counters ([sonic-net#2239](sonic-net/sonic-utilities#2239))
* Improve the way to check port type of RJ45 port ([sonic-net#2249](sonic-net/sonic-utilities#2249))

Signed-off-by: dprital <[email protected]>
preetham-singh pushed a commit to preetham-singh/sonic-utilities that referenced this pull request Nov 21, 2022
…lback-docker (sonic-net#2278)

#### What I did

Fix issues:
1. upgrade-docker shall not check STATE DB for those container which do not support warm mode
2. rollback-docker sometimes cannot really rollback the container because it cannot get the correct docker image tag

#### How I did it

1. for containers do not support warm mode, ignore STATE_DB check
2. use another command to get docker image tag

#### How to verify it

Manual test.

UT is not created for this change because there are many docker command running for these two CLIs, thus:
1. Mock so many docker command causes the test case very tricky. The case would be even more complicated than the CLIs.
2. UT cannot really test the command in case of so many docker command

Instead, a new sonic-mgmt test case is on the way to cover these two CLI. Will create PR for the sonic-mgmt new test case later.
@Junchao-Mellanox Junchao-Mellanox deleted the fix-upgrade-docker-1 branch March 6, 2023 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants